Skip to content

Conversation

@tykeal
Copy link

@tykeal tykeal commented Aug 21, 2025

Proposed change

This commit introduces three new services for the Schlage integration:

  • add_code: Allows users to add a new access code to a Schlage lock.
  • delete_code: Enables users to delete an existing access code from a
    Schlage lock.
  • get_codes: Retrieves all access codes associated with a Schlage
    lock.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @dknowles2, mind taking a look at this pull request as it has been labeled with an integration (schlage) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of schlage can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign schlage Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

This comment was marked as outdated.

@home-assistant home-assistant bot marked this pull request as draft September 1, 2025 14:55
@home-assistant
Copy link

home-assistant bot commented Sep 1, 2025

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@tykeal tykeal force-pushed the schlage_door_codes branch from bdbc056 to 460eb9f Compare September 5, 2025 14:22
@tykeal tykeal requested a review from Copilot September 5, 2025 14:22

This comment was marked as outdated.

@tykeal tykeal force-pushed the schlage_door_codes branch from 460eb9f to bb141ca Compare September 5, 2025 14:42
@tykeal tykeal requested a review from Copilot September 5, 2025 14:43

This comment was marked as outdated.

@tykeal tykeal force-pushed the schlage_door_codes branch 2 times, most recently from 554a45b to 336de4d Compare September 5, 2025 14:52
@tykeal tykeal requested a review from Copilot September 5, 2025 14:53

This comment was marked as outdated.

@tykeal tykeal force-pushed the schlage_door_codes branch from 336de4d to c9838d4 Compare September 5, 2025 15:01
@tykeal tykeal requested a review from Copilot September 5, 2025 15:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds three new services to the Schlage integration for managing door lock access codes: add_code, delete_code, and get_codes. These services enable users to programmatically manage PIN codes on their Schlage locks through Home Assistant.

Key changes:

  • New service implementations with validation for code names and values
  • Service definitions and localization strings
  • Comprehensive test coverage for all service scenarios
  • Integration with existing coordinator for data refresh

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
homeassistant/components/schlage/lock.py Implements the three new services with validation logic and entity service registration
homeassistant/components/schlage/services.yaml Defines service schemas and field configurations for the UI
homeassistant/components/schlage/strings.json Adds service descriptions and error message translations
homeassistant/components/schlage/icons.json Provides MDI icons for the new services
homeassistant/components/schlage/const.py Defines service name constants
homeassistant/components/schlage/coordinator.py Updates API call to include access codes in lock data
homeassistant/components/schlage/__init__.py Minor formatting change
tests/components/schlage/test_lock.py Comprehensive test coverage for all service scenarios including edge cases

@tykeal tykeal marked this pull request as ready for review September 5, 2025 15:10
@home-assistant home-assistant bot requested a review from joostlek September 5, 2025 15:10
@tykeal
Copy link
Author

tykeal commented Sep 5, 2025

@joostlek and @dknowles2 I've implemented the requested changes.

NOTE pytest is complaining that there is no translation defined for two things when the translation is defined. I don't know why it's doing that. My local test system uses the translations with no problems so I don't know why it's failing.

@dknowles2 I had to re-arrange some of the strings.json file as it was not passing hassfest locally.

@abmantis abmantis changed the title Feat: Add services for managing Schlage door codes Add services for managing Schlage door codes Oct 7, 2025
return {}

platform = entity_platform.async_get_current_platform()
platform.async_register_entity_service(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a new helper to register entity services from async_setup, which addresses the concerns joostlek pointed out: homeassistant.helpers.service.async_register_platform_entity_service

Can you refactor to use that one? It should not require significant changes to the services implementation.

See https://developers.home-assistant.io/docs/dev_101_services/#entity-service-actions

Copy link
Author

@tykeal tykeal Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abmantis I'm sorry, are you asking me to move the service registrations for 2 of the services up to the main schlage component, particularly when the services are calling private functions against the lock entity?

I say 2, because the 3rd one returns data and the documentation you linked still has me defining that one where I have it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abmantis I'm sorry, are you asking me to move the service registrations for 2 of the services up to the main schlage component, particularly when the services are calling private functions against the lock entity?

Yes. I would recommend removing the underscore prefix from the the function names because they are not private (even with the current implementation). They are passed to async_register_entity_service and called by the service helper, not by the entity class.

I say 2, because the 3rd one returns data and the documentation you linked still has me defining that one where I have it.

Not sure I understand what you mean. The docs on the Response data don't use async_register_entity_service. async_register_platform_entity_service also has support for returning data.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developers.home-assistant.io/docs/dev_101_services/#response-data <- the example for the response data is still shows it being done in async_setup_entry via async_rengister and not async_setup via async_register_platform_entity_service

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

developers.home-assistant.io/docs/dev_101_services#response-data <- the example for the response data is still shows it being done in async_setup_entry via async_rengister and not async_setup via async_register_platform_entity_service

That one is not an entity service like the ones in this PR. But that example needs to be updated too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: We should update the developer documentation. @abmantis do you have time? If not, I can open a PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abmantis I implemented in async_setup_entry as the integration doesn't do yaml based configuration, only config flow configuration and all the documentation I've read says async_setup is for yaml based configuration while async_setup_entry is for config flow based setup. If the integration supported yaml based configuration, then I would have stuck it there but it doesn't.

Copy link
Contributor

@emontnemery emontnemery Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abmantis is right, services should be registered in async_setup. can you please change it?
We recently changed the guidelines, and the documentation is outdated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll work through it when I can this weekend. I'm aware I already missed the cut-off to make 2025.11 :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the documentation, I only found one remaining example of registering services from async_setup_entry, this PR fixes that: home-assistant/developers.home-assistant#2855

@home-assistant home-assistant bot marked this pull request as draft October 7, 2025 17:17
This commit introduces three new services for the Schlage integration:
- `add_code`: Allows users to add a new access code to a Schlage lock.
- `delete_code`: Enables users to delete an existing access code from a
  Schlage lock.
- `get_codes`: Retrieves all access codes associated with a Schlage
  lock.

Signed-off-by: Andrew Grimberg <[email protected]>
* Move service registration to __init__.py and convert lock services to
  instance methods.
* Update services.yaml and strings.json to use target instead of
  entity_id.

Signed-off-by: Andrew Grimberg <[email protected]>
@tykeal tykeal force-pushed the schlage_door_codes branch from c9838d4 to 0309971 Compare October 16, 2025 21:05
@tykeal tykeal marked this pull request as ready for review October 16, 2025 21:06
@home-assistant home-assistant bot requested a review from abmantis October 16, 2025 21:06
await self.hass.async_add_executor_job(self._lock.unlock)
await self.coordinator.async_request_refresh()

# Door code services
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Door code services

return {}

platform = entity_platform.async_get_current_platform()
platform.async_register_entity_service(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to register them in async_setup_entry instead of async_setup as previously suggested?

mock_lock.access_codes = {"1": existing_code}

with pytest.raises(
ServiceValidationError, match="Code name 'test_user' already exists"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to match any string

Comment on lines +98 to +104
# code is required by voluptuous, the following is just to satisfy type
# checker, it should never be None
if code is None:
raise ServiceValidationError(
translation_domain=DOMAIN,
translation_key="schlage_code_required",
) # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# code is required by voluptuous, the following is just to satisfy type
# checker, it should never be None
if code is None:
raise ServiceValidationError(
translation_domain=DOMAIN,
translation_key="schlage_code_required",
) # pragma: no cover
if TYPE_CHECKING:
assert code is not None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that code is str, so this code should not be needed at all.

self._validate_code_value(codes, code)

access_code = AccessCode(name=name, code=code)
await self.coordinator.hass.async_add_executor_job(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await self.coordinator.hass.async_add_executor_job(
await self.hass.async_add_executor_job(

"""Delete a lock code."""
codes = self._lock.access_codes
if not codes:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this raise ServiceValidationError?

)

if not code_id_to_delete:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this raise ServiceValidationError?

if not code_id_to_delete:
return

if self._lock.access_codes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already check if codes is empty above, so we can remove the check here


if self._lock.access_codes:
await self.coordinator.hass.async_add_executor_job(
self._lock.access_codes[code_id_to_delete].delete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self._lock.access_codes[code_id_to_delete].delete
codes[code_id_to_delete].delete

"schlage_name_exists": {
"message": "A PIN code with this name already exists on the lock"
},
"schlage_name_required": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unused

@home-assistant home-assistant bot marked this pull request as draft October 23, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants